Repository navigation
feat(core): add shared request timing collector and middleware - #389
ChristianPavilonis wants to merge 15 commits into
Conversation
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds a generic per-request timing collector (RequestTimings<N, D>) and opt-in, insert-if-absent attachment middleware to edgezero-core, with a shared runtime scenario in the three WASM adapter contract suites. I found no correctness bugs. I'm requesting changes on two API-shape items that are cheap now but become breaking changes once Trusted Server pins a release tag. Neither touches Trusted Server's call sites at 2791ceb.
Findings
Blocking
- 🔧
TimingErrornot#[non_exhaustive]: inconsistent withEdgeError/KvError/ConfigStoreError, and adding it after the tag is breaking (request_timing.rs:17) - 🔧 Hoist
t0out of the mutex: makeselapsed()infallible and the origin structurally immutable;elapsed()currently fails under cross-thread contention (request_timing.rs:100)
Non-blocking
- 🤔 Generic lifecycle names:
headers_ready_total/request_elapsed/resp_bytesmirror TS internals (request_timing.rs:51) - 🤔 Cloudflare clock semantics: workerd's
performance.now()advances only across I/O, and the contract test runs in Chromium (docs/guide/middleware.md:169) - ♻️ Shared fixture via cross-crate
#[path], never run natively: Cloudflare/Fastly/Spintests/contract.rs - ⛏
RequestTimingslacksDebug;must_usemessage wording;with_excluded_pathsreplace semantics undocumented - ⛏ Guide: show paired type aliases (a generic mismatch silently yields no handle); drop the reference to an internal test file
- ⛏ Spec/plan status lines, unchecked tasks, local paths and "keep the PR draft" text are stale relative to the PR
- 🌱 Contention drop counter for native runtimes; config-driven exclusions; exposing the middleware-created handle to outer layers
Behavioral claims above were checked with throwaway probe tests against 9c03cc59: generic-mismatch silence, elapsed() failing under cross-thread contention, exclusion-list replacement, and no handle on the response.
📌 Out of Scope
- Deployed-provider smoke testing is still unchecked in the test plan. It matters most for Cloudflare, where the local harness doesn't use workerd's clock.
- The Axum adapter has no
tests/contract.rs, so the shared timing scenario doesn't run there.
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS (core: 484 unit tests, plus thecompile_faildoctest) - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - GitHub: all 22 checks green, including Cloudflare/Fastly/Spin WASM tests
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Second pass at 9c03cc59. It covers areas the earlier review didn't reach: registering the middleware from the manifest, how the handle interacts with State<T>, CI coverage of the compile-fail test, how strong the runtime test is, and documentation consistency. I checked each finding against the code or with a throwaway probe. I'm requesting changes so these land together with the two 🔧 items from the earlier review, before the release tag.
Findings
Inline:
- 🤔
edgezero.tomlcan't register the middleware: aconst fnconstructor fixes it (middleware.rs:86) - 🤔
State<RequestTimings<…>>returns a 500 that points atwith_state(docs/guide/middleware.md:226) - 🤔 CI never runs the compile-fail test (request_timing.rs:40)
- 🤔 Overlapping spans on one slot add up past wall-clock time (request_timing.rs:129)
- 🤔 The runtime test can't catch a stopped clock (tests/support/request_timing.rs:37)
- ⛏
span(slot)?in the guide doesn't compile in a handler (docs/guide/middleware.md:233) - ⛏ The
web_timeexample needs aweb-timedependency (docs/guide/middleware.md:27) - ⛏ Assertions that can't fail (middleware.rs:360, plus four more listed there)
- ⛏
TimingSnapshotlacks theD = ()default (request_timing.rs:45) - 🌱
PhaseSpanhas nofinish/cancel(request_timing.rs:260)
Not attached to a diff line:
-
⛏
configuration.mdnow contradicts the corrected middleware guide:docs/guide/configuration.md:49still says "Manifest-driven middleware are applied in order before routes". This PR corrected the same claim inmiddleware.md:61-62. Suggest reusing that sentence: "Routes are matched first. Middleware then run in registration order for a matched route; unmatched 404/405 requests bypass the middleware chain." -
🌱 The middleware fits only two of Trusted Server's four adapters: at
2791ceb, only Cloudflare and Spin register it.- Fastly creates its collector before the app is built, so it can time AppBuild (
main.rs:128-131). - Axum reimplements insert-if-absent plus the
/healthexclusion as a tower service outsideRouterService(timing.rs:78-99). That also covers 404/405 and gives it a final point to render headers.
This is the gap behind the earlier 🌱 about reaching the middleware-created handle after the router returns. An EdgeZero
Servicewrapper aroundRouterServicewould cover unmatched routes and give every adapter one place to finish timing. - Fastly creates its collector before the app is built, so it can time AppBuild (
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS - doctests (
--workspace --doc): PASS (1 compile-fail test, 13 ignored) - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - GitHub: 21/21 checks green
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
😃 Reviewed all 11 changed files at 9c03cc5, including focused collector/API and middleware/adapter reviews. No actionable defects found.
The implementation preserves existing collectors, keeps phase and payload updates under one mutex, and follows the documented contention, cancellation, and explicit lifecycle contracts.
Local verification passed:
- cargo fmt --all -- --check
- cargo clippy --workspace --all-targets --all-features -- -D warnings
- cargo test --workspace --all-targets
- cargo test -p edgezero-core: 484 unit tests, two integration tests, and the compile-fail doctest passed; 13 existing doctests ignored
- cargo check --workspace --all-targets --features "fastly cloudflare spin"
- cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin
- Fastly WASM contract suite under Viceroy: all seven tests passed
- Docs lint and formatting
Initial sandbox socket-binding and certificate-store failures cleared on rerun outside the sandbox. All 22 GitHub checks were green at the reviewed commit. Cloudflare/Spin runtime suites and Trusted Server joint validation were not rerun as part of this review.
Keep core doctests in the workspace host suite alongside main's CLI test matrix.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 878be472, which is ab444946 plus the merge of main. Both 🔧 items from the first review are fixed correctly. t0 now sits outside the mutex, so elapsed() is lock-free, and a cross-thread test covers it. TimingError and TimingSnapshot are #[non_exhaustive]. I checked every other claimed fix against the code and each one holds. The deferrals recorded in the spec's "Deferred follow-ups" are reasonable.
I'm requesting changes for the items below. The main ones are the Cloudflare/Spin gap in the guide's advice on where to measure, and the release gate on Trusted Server #1261.
Findings
Inline:
- 🤔 Cloudflare and Spin have no adapter hook for preinstalling a handle (docs/guide/middleware.md:288)
- ⛏ The doctest gate is missing from the local gate scripts and
CLAUDE.md(.github/workflows/test.yml:97) - ⛏ The spec and plan have stale consumer references, the wrong browser name and old evidence counts (spec:8)
- ⛏ Some process and authorization text remains (spec:478)
Not attached to a diff line:
- ⛏ The PR description is stale: it still says Trusted Server migration and joint verification are "complete against this exact candidate". The test plan's counts (484 unit tests; Cloudflare 8, Fastly 7, Spin 13) and the "22 checks … identical to
9c03cc59" note describe the original commit, and the public API has been renamed since then. The Changes table also leaves out.github/workflows/test.ymland theedgezero-macrosfixture. Suggest refreshing it and marking joint validation as pending, as the spec's "Remaining release checkpoints" already does. - 📝 The release gate isn't met yet: Trusted Server #1261 pins all six crates to
ab444946. Its Fastly adapter usesedgezero_adapter_fastly::lifecycle::{Sandbox, run_custom}, which comes from #379 and exists on this branch only after the merge. As a result its CI fails to compile (E0433 attrusted-server-adapter-fastly/src/main.rs:125). The spec's checkpoints put renewed joint validation before the EdgeZero merge, and that validation hasn't happened yet. #1261 needs to re-pin to878be472or later and pass.
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS - core doctests (
-p edgezero-core --doc): PASS (1 compile-fail test, 13 ignored) - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - GitHub: 28/28 checks green, including Cloudflare (10 tests, headless Firefox), Fastly/Viceroy (9) and Spin/Wasmtime (14)
…ression tests, synchronize local doctest gates, refresh verification records, and remove session-only narrative. Production behavior unchanged.
…t changing behavior.
|
@aram356 ready for another pass on the EdgeZero changes at Replied to and resolved the four inline threads. I also refreshed the PR description, including the missing workflow/macro fixture changes and the verification status. It no longer claims that the original joint validation covers this candidate. Your non-inline release-gate finding remains open. Trusted Server #1261 still pins all six crates to |
…delivery before closing the origin; add regression coverage.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 777e8edf: 4dd6bf82 and 01852979 on top of the previous revision, the unrelated harness fix 42f429ad, and three merges of main. All four threads from the last review are fixed correctly:
- The guide now names the boundary each adapter offers and adopts
TimingBoundarywith its limits, backed by three regression tests. - The doctest gate is in
run_tests.sh,/check-ciandCLAUDE.md. - The spec and plan cite #1261 and say Firefox.
- The process text is gone.
The refreshed PR description is clear about the open merge gate. I'm requesting changes because that gate is still unmet, and for the smaller items below.
Findings
Inline:
- ♻️ Release status tied to specific commits keeps going stale (plan:30)
- ⛏
stackpop/edgezerolinks remain after the IABTechLab transfer (spec:452) - ⛏ The Rust 1.95 compatibility shim on
PhaseSpan'sDropis no longer needed (request_timing.rs:287) - ⛏ An unrelated fixture-harness fix is bundled in (tests/fixtures/reusable-app/crates/fixture-harness/src/net.rs:160)
- ⛏ The
/check-ciandCLAUDE.mdgate lists disagree (.claude/commands/check-ci.md:1)
Not attached to a diff line:
- 📝 The merge gate is still unmet: Trusted Server #1261 still pins all six crates to
ab444946. It hasn't changed since 2026-10-19 and has 4 failing checks. The PR's own gate requires re-pinning to the current candidate and passing joint validation before this merges, so I'm not approving yet. - ⛏ The PR description has drifted slightly:
- "Current pushed candidate:
01852979…" should now be777e8edf. - The Changes table leaves out
tests/fixtures/reusable-app/crates/fixture-harness/src/{net,runners}.rs. - The Clippy line says "Rust 1.95 and 1.98.1", but main targets 1.98.1 only.
- "Current pushed candidate:
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings, Rust 1.98.1): PASS - tests (
--workspace --all-targets): PASS, including all 24 request-timing tests - core doctests (
-p edgezero-core --doc): PASS (1 compile-fail test, 13 ignored) - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - reusable-app fixture harness: PASS (25 tests)
- GitHub: 28/28 checks green at
777e8edf
…six local CI gates with regression coverage, canonicalized links, and simplified the Drop lint expectation.
Resolve middleware guide conflict, preserving request-timing semantics and base pre-dispatch guidance.
|
Closes #446. Tracks the shared request-timing collector and opt-in attachment middleware implemented by this PR. |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 511ce06f: 3b436c76 plus merges of main, including #403 (pre-dispatch hooks). Four of the five threads from the last review are fixed correctly:
- Release status has moved out of the spec and plan, which now link to the PRs instead.
- Every EdgeZero link in both documents points at
IABTechLab/edgezero. PhaseSpan'sDropuses main's plain#[expect(clippy::missing_trait_methods)]./check-cimatches the six documented gates.
The fixture-harness thread has no reply yet; see below. I'm requesting changes because the merge gate is still unmet, and for the items below.
Findings
Inline:
- 🤔 #403's pre-dispatch hook makes the Cloudflare/Spin boundary advice inaccurate (docs/guide/middleware.md:300)
- ♻️ The gate-docs guard sits in the CLI crate and hard-codes the gate count (crates/edgezero-cli/tests/ci_gate_docs.rs:1)
Not attached to a diff line:
- 📝 The merge gate is still unmet: Trusted Server #1261 still pins all six crates to
ab444946, through the oldstackpopURL. Its head9654deb7now conflicts with its ownmain, so only CodeQL ran there. No joint validation covers this candidate, so I'm not approving yet. - ⛏ The PR description is stale, and it's now the only status record: since
3b436c76moved release status out of the spec and plan, the description is where it lives. But:- "Current pushed candidate:
018529795767…" should now be511ce06f. - "Clippy compatibility follow-up in
01852979for Rust 1.95 and 1.98.1" no longer applies:3b436c76removed that shim. - "Historical local and post-merge verification is recorded separately in the implementation plan" points to records that
3b436c76deleted. - "Its current head,
686826ea…, has failing CI" should now name9654deb7, which conflicts with its ownmain. - The Changes table leaves out
crates/edgezero-cli/tests/ci_gate_docs.rsand the fixture-harness files.
- "Current pushed candidate:
- ⛏ Follow-up on the fixture-harness thread, with a correction: my last review said main's test workflow had passed on every recent run. Main's own branch runs did, but PRs into
maindo hit the race that42f429adfixes. Runs 40543103198 (a docs-only@types/nodebump), 41041682390 (sha2) and 41041668912 (tokio) all fail in the/stream-errorprobe with "not post-commit evidence". That makes the fix worth landing on its own now, rather than waiting behind this PR's release gate.
CI Status
- fmt: PASS
- clippy (
--all-features -D warnings, Rust 1.98.1): PASS - tests (
--workspace --all-targets): PASS, including every request-timing test and the new gate-docs test - core doctests (
-p edgezero-core --doc): PASS - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - reusable-app fixture harness: PASS (25 tests)
- GitHub: 28/28 checks green at
511ce06f
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 31b66d4b, which adds the hook guidance, the relocated gate guard and a merge of main (#435 only). The three threads answered since the last review hold up:
- Hook guidance: the guide now separates
PreDispatchHookinstallation fromTimingBoundary's marks on matched routes, and two new regression tests cover hook-only and hook-plus-boundary behavior. - Gate guard: the guard is now
scripts/check_ci_gate_docs.sh, run fromtest.ymlbeside the other repository checks, and it derives the displayed count. - Fixture fix: keeping
42f429adhere is fine for the reasons given.
The coordinated merge gate is now met (see below). I'm requesting changes for the macOS failure in the new test script and two smaller items.
Findings
Inline:
- 🔧
scripts/test_ci_gate_docs.shfails on macOS's BSD awk (scripts/test_ci_gate_docs.sh:27) - ⛏ The new Mermaid diagram renders as raw source on the published site (docs/guide/middleware.md:310)
Not attached to a diff line:
- 📝 The merge gate is met for this pair: Trusted Server #1261 at
ee249d36pins all six crates to31b66d4bthroughIABTechLab/edgezero. Its lockfile resolves exactly one copy of each EdgeZero crate from that revision, and all 29 of its checks pass, including browser integration, integration tests, cross-adapter parity and the Fastly EC lifecycle.mainhasn't moved sincecf2c57b1, the base of this branch's last merge, so merging now keeps the jointly validated tree. Tagging, re-pinning Trusted Server to the tag and tag-backed validation remain as post-merge steps. - ⛏ The PR description lags behind that: it still says the consumer's "Fresh GitHub jobs … are still running against this pushed pair" and that "the merge gate remains open", and the "Fresh GitHub checks and complete coordinated consumer validation" box is unchecked. Suggest recording that the
31b66d4b/ee249d36pair passed (28/28 and 29/29), and noting that any further push or merge ofmainbreaks the pairing.
CI Status
check_ci_gate_docs.sh: PASStest_ci_gate_docs.sh: FAIL on macOS (BSD awk); see the inline comment. CI's Linux run passes.- fmt: PASS
- clippy (
--all-features -D warnings, Rust 1.98.1): PASS - tests (
--workspace --all-targets): PASS, including all 30 request-timing tests - core doctests (
-p edgezero-core --doc): PASS - feature check (
fastly cloudflare spin): PASS - Spin
wasm32-wasip2check: PASS - reusable-app fixture harness: PASS (25 tests)
- docs build (
vitepress build): PASS - GitHub: 28/28 checks green at
31b66d4b
|
@aram356 ready for another pass at 0653e6e3. The two latest inline findings are fixed, with details and checks in their thread replies. The non-inline status requests in your latest review are reflected in the PR description:
Earlier non-inline points remain accounted for: the configuration guide states that route matching precedes middleware; the fixture-harness repair stays in this PR as agreed in the latest review and is listed in the description; candidate status lives in the PR rather than the design records. An optional outer-service wrapper, response propagation, contention diagnostics, config-driven exclusions, explicit span finish/discard and an optional timing extractor remain deferred with reasons in the spec. The pre-dispatch installation guidance is implemented; after-dispatch result observation remains deferred. Deployed-provider clock smoke testing is still unverified. Local checks for the committed follow-up passed: the six-command docs guard; all 14 shell regressions under GNU awk and upstream awk |
Summary
Changes
crates/edgezero-core/src/request_timing.rs,src/lib.rsRequestTimings<N, D>, immutable lock-free origin, snapshots, compound updates, saturating accumulation, drop-time spans and explicit lifecycle markscrates/edgezero-core/src/middleware.rscrates/edgezero-core/tests/request_timing_boundary.rscrates/edgezero-macros/tests/app_macro.rs.github/workflows/test.yml,scripts/run_tests.sh,.claude/commands/check-ci.md,CLAUDE.mdscripts/check_ci_gate_docs.sh,scripts/test_ci_gate_docs.shtests/fixtures/reusable-app/crates/fixture-harness/src/{net,runners}.rs42f429ad: wait for downstream body observation before origin closure and retain strict partial-response/post-commit-error assertionsdocs/guide/assets/request-timing-flow.svgand design/planelapsed()is infallible and does not take the state mutex.TimingErrorandTimingSnapshotare non-exhaustive. Lifecycle methods aremark_headers_ready(),mark_request_complete()andset_response_bytes().Callbacks run synchronously under one nonblocking mutex. Panics propagate; poison recovery is best-effort, without rollback or payload validation. Trusted Server retains its phase enum, domain facts, wire names and
Server-Timingrendering.PreDispatchHookcan install or reuse the handle on every adapter before route lookup, including 404/405 requests. It cannot observe downstream results. Application boundary middleware marks matched-route results, before handler-error rendering, and does not measure streamed-body completion. The guide covers single-hook composition, manualHooksintegration forapp!, and exclusions at every creation site. Existing handles retain their original clock origin.Verification
Current pushed EdgeZero candidate:
0653e6e383673f2032160c9af73a7c5e5d32836b.The previous jointly validated pair is EdgeZero
31b66d4b/ Trusted Serveree249d36. The current head adds only the awk fixture-helper portability fix and rendered timing diagram; it does not change Rust source or timing behavior.Full local checks at the preceding candidate
31b66d4b, Rust 1.98.1, with Rust inputs unchanged by this follow-up:fastly cloudflare spinfeature check, and Spinwasm32-wasip2check.31b66d4b/ Trusted Serveree249d36. All 29 reported checks in each repository passed, including runtime/browser integration acceptance. This evidence applies to that pair, not the new head or a future merge tree.0653e6e3. Trusted Server still pins31b66d4b; update the integration pins/lockfile and record refreshed pairing evidence before merge.The spec and implementation plan retain design contracts, verification approach and generic release checkpoints. Candidate revisions and validation status live in the coordinated PRs, not those lasting design records.
Review follow-up in 0653e6e3:
scripts/test_ci_gate_docs.shtransfers replacement strings throughENVIRON, and the middleware guide uses a checked-in SVG instead of an unsupported Mermaid code block. All 14 shell cases pass under GNU awk and upstream awk20200816compiled on Linux; native macOS was not exercised. Bash syntax, ShellCheck 0.11.0, docs formatting/lint/build, Cargo formatting and all five timing-boundary regressions pass. The built HTML embeds the exact SVG with descriptive alternative text. These local checks cover the committed follow-up; the previous pair's CI results do not establish joint validation of this new head.Coordinated delivery / merge gate
Trusted Server #1261 remains at
ee249d363e52227e6fabf134c436e90c6d3df2f8. All six direct EdgeZero dependencies use the IABTechLab URL and pin the previous jointly validated candidate31b66d4b1c48dd0bf4a600837268d7eb7e3fb5b3, not the new head above. Its integration pin update changed only the eight resolved EdgeZero git-source revisions; metadata confirms one resolved EdgeZero core identity and one common git source. The next coordinated candidate pin and renewed joint validation remain pending.Local consumer validation passed the 20 timing facade tests; Axum, Cloudflare and Spin adapter suites; Fastly/Viceroy tests in both feature modes, including 3,414 core tests; 28 cross-adapter parity tests; host CLI tests; all eight target-matched Clippy aliases; template build-digest tests/Clippy; the WASM clock lint; formatting and the CI Node pin guard. The full JS suite passed 1,892 tests with one skipped test and no type errors. Exact consumer validation commands and results.
Previous candidate-specific joint validation passed for EdgeZero
31b66d4b/ Trusted Serveree249d36: EdgeZero checks and consumer checks are green, including browser integration, cross-adapter parity and the Fastly EC lifecycle. The consumer's older PR-description warnings about missing lifecycle APIs are stale relative to its current pins and passing checks.EdgeZero
mainhas since advanced fromcf2c57b1toe11ea308via #456, changing onlydocs/package-lock.json; this branch is now behindmain. The previous pair passed its validation gate, but those runs do not establish validation of0653e6e3or a future merge tree. This push, any integration ofmain, and any consumer change require refreshed candidate-specific pairing evidence before merge.Trusted Server must replace temporary revision pins with an approved EdgeZero release tag, regenerate and inspect its lockfile, and pass final tag-backed validation before merging. No merge or release is performed by this follow-up.
Related work
Closes #446.